sequenceDiagram
  participant Client
  participant DialogRoot
  participant Presence
  participant DialogContentModal
  participant DialogContentNonModal
  participant DialogOverlayImpl
  Client->>DialogRoot: mount with unmountOnHide (true|false)
  DialogRoot->>Presence: provide :present/:force-mount (computed)
  Presence->>DialogContentModal: present slot value
  Presence->>DialogContentNonModal: present slot value
  Presence->>DialogOverlayImpl: present slot value
  DialogContentModal->>DialogContentModal: use present to set aria-hidden target & pointer events
  DialogContentModal->>DialogContentModal: watch present -> restore focus when false
  DialogOverlayImpl->>DialogOverlayImpl: watch present -> toggle body scroll lock

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Open in StackBlitz

npm i https://pkg.pr.new/reka-ui@2662

commit: 515a0b2

You dont believe how happy you make me with this PR! Will test soon, hope it gets merged eventually...

Thanks for the PR @benjamincanac ! Main concerns are in the two shared primitives it touches.

🔴 Major

1. DismissableLayer change is stale / conflicts with #2674. This branch forked before 034d20edd (#2674) landed on v2, which already rewrote this to a watch and solves the same "restore body pointer-events when toggled off while mounted" problem. The toRaw/watchEffect change will merge-conflict and is redundant → rebase onto v2 and drop it (keep the new tests).

2. New FocusScope MutationObserver is added to every consumer. Gated only on isClient, not on force-mount, so Popover/Menu/Select/Combobox/Toast all now get a style/class observer calling getComputedStyle().display on every mutation — forced reflow during animations, plus possible focus-stealing on display: none → visible transitions. → Scope it to the force-mounted-hidden case.

🟡 Medium

• display: none sniffing is brittle as a primitive-level contract (breaks for visibility/opacity/hidden). Prefer an explicit "visible" signal from the consumer.
• Duplicated trigger-refocus watch in both DialogContentModal/NonModal, now overlapping FocusScope's new re-focus → double-focus/race risk on rapid toggling.
• v-show vs hidden="until-found" — existing unmountOnHide primitives (Collapsible/Tabs/Accordion) use until-found, which is more SEO-discoverable than display: none. Worth a note on the choice.

🟢 Looks good

aria-hidden gating, scroll-lock watch (valid — useBodyScrollLock returns a writable ref), present-prop stripping, typo fix. Minor: setTimeout(r, 10) magic waits in tests are a CI-flakiness risk.

Recommendation: resolve #1 (rebase + drop) and #2 (scope the observer) before merge; 3–5 worth addressing or documenting.

Thanks for the review @zernonia! Everything should be addressed in the latest push:

1. Rebased on v2, the toRaw/watchEffect change is gone and the regression tests are kept (they pass against your [Bug]: Dropdown inside dialog strips pointer-events style from body #2674 implementation). DismissableLayer is touched again but for a new reason, see below.
2. Removed the MutationObserver entirely. FocusScope now takes an explicit present prop, consumers that don't pass it are unaffected.
3. No more display: none sniffing, replaced by the explicit present signal as you suggested.
4. No overlap in practice: the Dialog watches only fire on close (restore trigger focus) while the FocusScope watcher only fires on open (auto-focus) and bails if focus already landed inside. There is a test asserting openAutoFocus fires exactly once per open.
5. v-show is deliberate: hidden="until-found" reveals content on beforematch without the overlay, focus trap or scroll lock engaging, which is fine for inline content (Collapsible/Tabs) but broken for overlay primitives. display: none content is still indexed as long as it's in the DOM, which is the actual SEO requirement from [Feature]: Dont remove Dialog from DOM (use v-show instead of v-if) #1727.

While testing in a real browser I found that both global stacks were keyed on mount: with two unmountOnHide: false dialogs on one page, the hidden one swallowed Escape / outside dismiss for the open one, and its hidden FocusScope paused the open modal's focus trap. Both primitives now key stack membership on present (same plumbing as 2., one level deeper), with regression tests.

Re-review (after 3ac782e)

Both 🔴 blockers are resolved — and well. Verified locally on f870a8623: Dialog + DismissableLayer + FocusScope (50 passed), and the other shared-primitive consumers Popover/Menu/Select/Combobox/Toast (89 passed), so the default path is confirmed unaffected.

✅ Resolved

• Feat: Accordion #1 DismissableLayer — rebased onto v2 ([Bug]: Dropdown inside dialog strips pointer-events style from body #2674 now in base) and the stale toRaw/watchEffect change is gone. The new internal present prop gating layer-stack membership + outside-interaction handling is the right model, and keeping stack membership in a separate watch from the pointer-events watch (so a disableOutsidePointerEvents toggle never re-orders the stack) is a nice touch.
• Feat: Alert Dialog #2 FocusScope — the unconditional MutationObserver/getComputedStyle is replaced by an explicit present prop (default true). Default consumers never enter the new watch(() => props.present) path, so no per-instance observer or forced reflow. This also resolves medium Feat: Aspect Ratio #3 (no more display:none sniffing — driven off an explicit signal). The dependency-tracking note (reading props.present after await so the mount effect doesn't track it) is correct.
• medium Feat: Avatar #4 — the close-path focus restore is now solely the Dialog modal/non-modal watch(present); FocusScope's present-watch only removes from the stack on close and re-focuses on open, so there's no double focus-restore. Stack add/remove is balanced across mount/open/close/unmount.

🟡 Minor (non-blocking)

1. Escape isn't gated on present. pointerDownOutside/focusOutside now early-return on !props.present, but the onKeyStroke('Escape') handler doesn't. A hidden layer's index is -1 so it's mostly neutralized, but when no layer is visible (layers.size === 0), index === size - 1 becomes -1 === -1 → true, so a closed-but-mounted Dialog still emits escapeKeyDown (and a no-op dismiss) on Escape. Worth gating Escape on props.present for consistency.
2. Redundant unmount cleanup. The trailing watchEffect that deletes from layers/layersWithOutsidePointerEventsDisabled on unmount now overlaps the two present-gated onCleanups. Harmless (idempotent), but could be dropped to reduce surface.
3. v-show vs hidden="until-found" (prior Feat: Checkbox #5) still stands — existing unmountOnHide primitives use until-found, which is more discoverable for the SEO case cited. Fine to keep v-show if intentional; just worth a one-line rationale.

Nice iteration — happy to see this land once the Escape nit is considered.

Pushed 515a0b2 directly to the branch addressing minor #1 — gates the Escape handler on props.present (consistent with the pointerDownOutside/focusOutside guards) so a hidden, mounted layer no longer emits escapeKeyDown/dismiss when no layer is visible. Added two regression tests; full DismissableLayer + Dialog/AlertDialog/Popover/Menu suites pass locally. Feel free to squash/reword. Left #2 (redundant unmount cleanup) and #3 (v-show vs until-found) for you to decide on.